Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to slog #240

Merged
merged 1 commit into from
Sep 9, 2024
Merged

Switch to slog #240

merged 1 commit into from
Sep 9, 2024

Conversation

lucacome
Copy link
Contributor

@lucacome lucacome commented Aug 31, 2024

Switch to log/slog to align with prometheus/common#677

@lucacome lucacome marked this pull request as ready for review August 31, 2024 00:33
@SuperQ SuperQ self-requested a review August 31, 2024 03:51
@SuperQ
Copy link
Member

SuperQ commented Sep 1, 2024

At a minimum, I'm going to cut a new release before merging this. (#246)

@SuperQ
Copy link
Member

SuperQ commented Sep 3, 2024

@roidelapluie What do you think? Should we just make this a breaking change? Or should we make a new compatible function?

@roidelapluie
Copy link
Member

@roidelapluie What do you think? Should we just make this a breaking change? Or should we make a new compatible function?

I think a new compat function would be better. A lot of repos use this.

@lucacome
Copy link
Contributor Author

lucacome commented Sep 3, 2024

Aren't breaking changes kind of expected for this repo?

I feel like both the version 0.x.x and the note that is still in the README signal that

This repository is currently WIP and experimental.

I also feel like it's not that hard of a change to make for users and it would reduce the number of dependencies.
But I'm happy to make changes to use a compact function, if you think that's better.

Signed-off-by: Luca Comellini <luca.com@gmail.com>
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go ahead with this as a breaking change. It's an opinionated library used mostly by us and our exporters.

If we get a lot of complaints, we can make a decision to revert.

We currently have the people and the momentum to migrate stuff. I'm planning to publish a helper script for converting go-kit/log calls to slog.

@SuperQ
Copy link
Member

SuperQ commented Sep 9, 2024

@roidelapluie what do you think?

@roidelapluie
Copy link
Member

OK!

@SuperQ SuperQ merged commit d0e3731 into prometheus:master Sep 9, 2024
4 checks passed
SuperQ added a commit that referenced this pull request Sep 9, 2024
* [CHANGE] Switch logging library to slog #240

Signed-off-by: SuperQ <superq@gmail.com>
@SuperQ SuperQ mentioned this pull request Sep 9, 2024
@lucacome lucacome deleted the slog branch September 9, 2024 14:52
SuperQ added a commit that referenced this pull request Sep 9, 2024
* [CHANGE] Switch logging library to slog #240

Signed-off-by: SuperQ <superq@gmail.com>
@dswarbrick
Copy link
Contributor

dswarbrick commented Sep 11, 2024

Despite this package being pre-1.0 and thus entitled to make breaking changes, this will nevertheless be quite inconvenient for packaging in Debian & derivatives, due to their lack of support for multiple concurrent versions of libraries.

Packaging exporter-toolkit v0.13.0 will break approximately 25 exporters in Debian which currently depend on it. Patching all 25 exporters will be considerable work, not to mention the turnaround time of getting all those patches pushed to their respective upstream projects.

I have put some thought into how exporter-toolkit could have made this change non-breaking, and came up with this:

func ListenAndServe(server *http.Server, flags *FlagConfig, origLogger any) error {
	var logger *slog.Logger

	switch origLogger := origLogger.(type) {
	case log.Logger:
		// Re-parsing the command line is the only way to determine the go-kit log config, since
		// nothing useful is exported by go-kit/log.Logger.
		// Since promslog uses exactly the same flag names and options as promlog, we can take a
		// shortcut and parse the args directly into a promslog.Config.
		app := kingpin.New("", "")
		config := &promslog.Config{Style: promslog.GoKitStyle}
		flag.AddFlags(app, config)
		kingpin.MustParse(app.Parse(os.Args[1:]))
		logger = promslog.New(config)
	case *slog.Logger:
		logger = origLogger
	default:
		panic("unsupported logger type")
	}

...

As a maintainer of the golang-github-prometheus-exporter-toolkit package in Debian, I will implement this as a patch to ease the transition for all the "legacy" exporters which still use go-kit/log. However, if there is interest here, I can submit a PR to have this workaround implemented here for the benefit of all.

@SuperQ
Copy link
Member

SuperQ commented Sep 14, 2024

We're planning to be rolling out the exporter-toolkit logging change reasonably quickly and cutting new exporter releases. If we did this over the next ~month, would that be reasonable for updating Debian?

For example, I've already done this in the node_exporter (prometheus/node_exporter#3097).

Otherwise, we could implement the any handler here.

@dswarbrick
Copy link
Contributor

Debian packages a lot of third-party exporters that are not under the maintenance umbrella of the Prometheus community. Some of these exporters rarely get updated by their developers. I have personally opened quite a few PRs against the upstream projects, nudging them to modernize, so that Debian does not have to maintain such a large patch set. One such example was the switch from logrus to go-kit logging just a few years ago.

Whilst I have faith that you can update many of the core Prometheus exporters in a reasonable timeframe, my concern is mainly the multitude of third-party exporters, some of which are even borderline unmaintained.

@lucacome
Copy link
Contributor Author

@dswarbrick do you have a list of those projects? I wouldn’t mind helping out.

@dswarbrick
Copy link
Contributor

@lucacome Not specifically, but https://packages.debian.org/search?suite=sid&keywords=prometheus is a good starting point. Note that not all of the exporters are written in Go, but I reckon probably about two thirds of them are, and of those, the vast majority use exporter-toolkit.

@dswarbrick
Copy link
Contributor

A couple of other things that have since caught my eye about this change, which may cause minor issues with users who have configured log parsers (e.g. Loki) to read exporter logs:

  • The ts (timestamp) field is now time, and is expressed as a local time with timezone offset. Previously it was expressed as UTC.
  • The caller field is now source, and is more verbose, with a fully qualified path.
  • The level field values are now capitalized, e.g. "INFO"

@SuperQ
Copy link
Member

SuperQ commented Sep 19, 2024

@dswarbrick Yes, those changes are intentional. We want to switch to slog. So switching to slog's standard formatting is the default. The upstream promslog allows for a backwards compatible format, but we decided not to use it here, as this is an opinionated library.

@dswarbrick
Copy link
Contributor

dswarbrick commented Sep 19, 2024

@lucacome I just built a local golang-github-prometheus-exporter-toolkit 0.13.0 package for Debian, and used ratt to identify reverse dependencies and check their builds.

Of the 23 packages in sid which currently depend on exporter-toolkit, 22 of them failed to build (unsurprisingly).

2024/09/19 17:27:42 Build results:
2024/09/19 17:27:42 PASSED: alertmanager-irc-relay
2024/09/19 17:27:42 FAILED: prometheus-pgbackrest-exporter (see buildlogs/prometheus-pgbackrest-exporter_0.18.0+ds1-1)
2024/09/19 17:27:42 FAILED: prometheus-mqtt-exporter (see buildlogs/prometheus-mqtt-exporter_0.1.7-1)
2024/09/19 17:27:42 FAILED: prometheus-mysqld-exporter (see buildlogs/prometheus-mysqld-exporter_0.15.1-1)
2024/09/19 17:27:42 FAILED: prometheus-alertmanager (see buildlogs/prometheus-alertmanager_0.27.0+ds-2)
2024/09/19 17:27:42 FAILED: prometheus-blackbox-exporter (see buildlogs/prometheus-blackbox-exporter_0.25.0-1)
2024/09/19 17:27:42 FAILED: prometheus-ipmi-exporter (see buildlogs/prometheus-ipmi-exporter_1.8.0-1)
2024/09/19 17:27:42 FAILED: prometheus (see buildlogs/prometheus_2.45.6+ds-1)
2024/09/19 17:27:42 FAILED: prometheus-hacluster-exporter (see buildlogs/prometheus-hacluster-exporter_1.3.3-1)
2024/09/19 17:27:42 FAILED: prometheus-elasticsearch-exporter (see buildlogs/prometheus-elasticsearch-exporter_1.7.0-2)
2024/09/19 17:27:42 FAILED: prometheus-postgres-exporter (see buildlogs/prometheus-postgres-exporter_0.15.0-3)
2024/09/19 17:27:42 FAILED: prometheus-pushgateway (see buildlogs/prometheus-pushgateway_1.7.0-2)
2024/09/19 17:27:42 FAILED: prometheus-smokeping-prober (see buildlogs/prometheus-smokeping-prober_0.8.1-3)
2024/09/19 17:27:42 FAILED: prometheus-haproxy-exporter (see buildlogs/prometheus-haproxy-exporter_0.15.0-2)
2024/09/19 17:27:42 FAILED: prometheus-snmp-exporter (see buildlogs/prometheus-snmp-exporter_0.26.0-1)
2024/09/19 17:27:42 FAILED: prometheus-apache-exporter (see buildlogs/prometheus-apache-exporter_1.0.7-1)
2024/09/19 17:27:42 FAILED: prometheus-bind-exporter (see buildlogs/prometheus-bind-exporter_0.7.0-3)
2024/09/19 17:27:42 FAILED: prometheus-squid-exporter (see buildlogs/prometheus-squid-exporter_1.12.0-1)
2024/09/19 17:27:42 FAILED: prometheus-process-exporter (see buildlogs/prometheus-process-exporter_0.8.3-1)
2024/09/19 17:27:42 FAILED: prometheus-node-exporter (see buildlogs/prometheus-node-exporter_1.8.2-1)
2024/09/19 17:27:42 FAILED: golang-github-prometheus-community-pgbouncer-exporter (see buildlogs/golang-github-prometheus-community-pgbouncer-exporter_0.9.0-1)
2024/09/19 17:27:42 FAILED: prometheus-nginx-exporter (see buildlogs/prometheus-nginx-exporter_1.3.0-1)
2024/09/19 17:27:42 FAILED: prometheus-frr-exporter (see buildlogs/prometheus-frr-exporter_1.3.1-1)

I will go ahead with my plan to patch exporter-toolkit in Debian so that it will accept a legacy go-kit logger as an interim workaround, as I don't currently have the time to migrate 22 exporters to log/slog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants